replace manage facility admin screen dropdowns with combos #6439

fzhao99 · 2023-08-28T20:10:14Z

FRONTEND PULL REQUEST

Related Issue

Part one of #6393

Will follow up with the unarchive page in a separate PR.

Changes Proposed

Refactor ManageFacility component to use Truss combo boxes rather than our custom dropdown
Update tests accordingly
Added a new custom ComboBox based on the Truss version with an added required prop

Testing

deployed on dev5

Before

After

Screen.Recording.2023-08-31.at.10.24.27.AM.mov

Org selection enables facility selection

Screen.Recording.2023-08-31.at.10.26.26.AM.mov

Clear filters on individual combos cascade

Screen.Recording.2023-08-31.at.10.19.38.AM.mov

Clear filter button clears both boxes

Screen.Recording.2023-08-31.at.10.20.21.AM.mov

Clear org resets search results

Screen.Recording.2023-08-31.at.10.27.19.AM.mov

Additional Information

TL;DR: Are we ok sticking with fireEvent / wrapping things in act like we're doing currently, or do we think it's worth it to try and prefer userEvent / clean act warnings in the future? The issue is that using fireEvent / suppressing act warnings with the extra tag may result in tests that don't catch the behavior they were written for, as well as resulting in more stable tests because we're accounting for all the state updates that are happening within our component (to the extent React can tell us). On the flip side though, going back and refactoring might be expensive / unfun. I reworked some of the tests in the above fashion here in case more concrete examples are helpful.

If yes, happy to make a ticket and prioritize accordingly alongside the other tech debt work we have in the pipeline. Otherwise, we can revisit if it ever comes up again.

The issue

In getting this PR in I discovered a couple of issues with the act warning React is throwing based on the way that we're setting up our testing suite. I'll outline what I've found here and would love opinions on options to move forward!

As we've addressed in PR's like this one, act warnings pop up periodically in our component testing. These are often useful warnings React offers to notify us about unhandled state changes within our components that might cause unintended side effects.

For this PR specifically, we need to use the userEvent API rather than the lower-level fireEvent to get tests to pass because the combo box is listening for a couple different events to happen in sequence in order to update. This caused a bunch of act warnings to pop up and I wanted to present a few options of handling them so we don't unnecessarily clutter our app logs and write more robust tests.

Option 0 - Use fireEvent

For the reasons stated above, we can't exactly use fireEvent here because the component we're testing for relies on a bunch of bundled events working together, but we do use this method extensively in other parts of the codebase. RTL specifically recommends against this option though, so wanted to call it out as something to potentially move away from so we're testing things in a way more in line with how they recommend.

Option 1 - Wrap the userEvents in `act`

One way of suppressing the warnings is to wrap the events in an act call. The RTL maintainer again recommends against this because the API calls already have act wrappers in them and the warnings might be pointing to actual state updates that need addressing. Back when Nick was on the project he also submitted a PR that called this out and installed an eslint plugin specifically for this issue. The plugin seems not to be working correctly because it's not yelling at us in instances where it should be.

I've opted to temporarily go this direction to get this PR in, but will mention that this is a bandaid solution since (most) of the act warnings are trying to tell us that we have side effects in our component code that could result in more brittle tests.

Option 2 - Update dependencies that are causing the issue in the first place and refactor our testing suite

This is, at least in my mind, the "correct" solution, but also the one that will take a bit of work. By using the higher-level interaction API and making sure we're handling all the act warnings, we're ensuring that we're covering any side effects that happen within our event handlers / component rendering logic. This video outlines a scenario where the act warning is trying to tell you something useful.

In order for us to to take advantage of userEvent without act warnings though, we need to make sure that the underlying shared @testing-library/dom dependency resolves to the same version.. I started exploring this on a separate branch but updating the packages breaks some other tests, so I punted on this for now. If we're in agreement that we want to do this, we'll need to go back and update some old tests so that everything is up to standard (and fix the eslint plugin in the meantime so that it's enforcing the new set of API's).

cypress/e2e/11-support-manage-facility.cy.js

fzhao99 · 2023-08-31T17:10:56Z

cypress/e2e/11-support-manage-facility.cy.js


+  beforeEach(() => {


moving this hook into a before each as recommended here

fzhao99 · 2023-09-05T17:29:58Z

Spike ticket for the act pieces made here

emyl3

LGTM! Thank you for doing this (combo boxes are so tricky 😭) and doing the initial research into the act warnings!!!

johanna-skylight

I left some questions/comments. Also I was not able to see the code to migrate the unarchive patient page. Is that going to be addressed in another PR or was it already migrated?.

Thanks for your work on integrating the trusswork component and sorry for the delayed review!

johanna-skylight · 2023-09-06T13:02:46Z

frontend/src/app/supportAdmin/ManageFacility/FacilitySelectFilter.tsx

              disabled={loading}
-              required={true}


The required true was needed to pass a11y scan. Does the combobox handle the required in a different way?

Truss doesn't give us a required prop in their out-of-the-box component, so I added our custom required UI in the heading piece. The automated scan with non-pro Axe tools didn't surface any issues but if you see one with your tools lmk and I'm happy to try and fix it.

Worth calling out that we might want to add new component we can extend with things like the required status since I'm guessing this won't be the last time this comes up. If folks are in agreement that this is useful happy to refactor the code a bit to make that extensible

@BobanL do you remember how you found the issue 6408?. Want to make sure we don't need the required at the input level before resolving this combo! :D

This happens when you test this as a form. I ran through it and it looks like it's still being flagged. I think for the purpose of accessibility we might need to figure out how to add required.

oh actually Truss gives us an escape hatch for this. Will push a fix soon

The latest commit adds to prop but if someone could run the scan for me again would appreciate it!

Tested it with axe and didn't find any problems!

When I ran it on the latest changes I saw this error but am ignoring because I believe this is a false positive jsyk

johanna-skylight · 2023-09-06T13:06:16Z

frontend/src/app/supportAdmin/ManageFacility/ManageFacility.tsx

-      );
-      handleClearFilter();
-    });
+    if (localState.facilityId) {


why is this condition needed? in theory the delete button will display only if a facility has been loaded

this is to make Typescript happy because I changed the typing of localState.facilityId to maybe be undefined (in the event nothing is selected) and the delete mutation hook doesn't like that. I thought having a conditional check here was preferable over casting since this delegates the type checking to the compiler rather than relying on a UI state to ensure facilityId is defined appropriately.

sonarcloud · 2023-09-06T20:03:10Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

94.1% Coverage
0.0% Duplication

BobanL

Looks good to me!

emyl3

Thank you for all your work on these tricky tricky combo boxes 🎃

johanna-skylight

👍

fzhao99 force-pushed the bob/6393-replace-admin-dropdowns-with-combos branch from acfe25c to 64c115e Compare August 29, 2023 16:23

fzhao99 changed the title ~~Bob/6393 replace admin dropdowns with combos~~ replace manage facility admin screen dropdowns with combos Aug 31, 2023

fzhao99 added 6 commits August 31, 2023 10:01

refactor using truss

8c8fdb1

add tests

c972ca6

temporarily wrap things in act

a285c5c

fix accessibility violations and refactor tests

3497425

scope down unnecessary changes

d9b61f0

use recommended userEvent setup pattern

20c9677

fzhao99 force-pushed the bob/6393-replace-admin-dropdowns-with-combos branch from 2e16c23 to 20c9677 Compare August 31, 2023 14:02

fzhao99 temporarily deployed to Dev5 August 31, 2023 14:13 — with GitHub Actions Inactive

fzhao99 added 4 commits August 31, 2023 10:15

fix mocks

c424b12

move gql alias to before each hook

b449eef

switch selector

397f579

rework e2e to work with combo box

641eb26

fzhao99 commented Aug 31, 2023

View reviewed changes

cypress/e2e/11-support-manage-facility.cy.js Outdated Show resolved Hide resolved

fzhao99 commented Aug 31, 2023

View reviewed changes

fzhao99 marked this pull request as ready for review August 31, 2023 17:43

fzhao99 requested review from mehansen, BobanL, emyl3 and johanna-skylight August 31, 2023 17:44

add org util query to simplify selectors

831d535

fzhao99 mentioned this pull request Sep 5, 2023

Triage frontend act warnings #6491

Closed

emyl3 previously approved these changes Sep 5, 2023

View reviewed changes

johanna-skylight suggested changes Sep 6, 2023

View reviewed changes

fzhao99 requested a review from johanna-skylight September 6, 2023 13:26

fzhao99 added 3 commits September 6, 2023 14:54

add extracted combo box component

9130d59

add forwardRef hook

54ed7ed

relax label selectors

4a21a24

fzhao99 dismissed emyl3’s stale review via 4a21a24 September 6, 2023 19:47

fzhao99 temporarily deployed to Dev5 September 6, 2023 19:59 — with GitHub Actions Inactive

fzhao99 requested a review from emyl3 September 6, 2023 20:37

BobanL approved these changes Sep 7, 2023

View reviewed changes

emyl3 approved these changes Sep 7, 2023

View reviewed changes

johanna-skylight approved these changes Sep 7, 2023

View reviewed changes

fzhao99 added this pull request to the merge queue Sep 7, 2023

Merged via the queue into main with commit 8b99a27 Sep 7, 2023
37 checks passed

fzhao99 deleted the bob/6393-replace-admin-dropdowns-with-combos branch September 7, 2023 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace manage facility admin screen dropdowns with combos #6439

replace manage facility admin screen dropdowns with combos #6439

fzhao99 commented Aug 28, 2023 •

edited

Loading

fzhao99 Aug 31, 2023

fzhao99 commented Sep 5, 2023

emyl3 left a comment

johanna-skylight left a comment

johanna-skylight Sep 6, 2023

fzhao99 Sep 6, 2023

johanna-skylight Sep 6, 2023

BobanL Sep 6, 2023

fzhao99 Sep 6, 2023

fzhao99 Sep 6, 2023

BobanL Sep 6, 2023

emyl3 Sep 7, 2023

johanna-skylight Sep 6, 2023

fzhao99 Sep 6, 2023 •

edited

Loading

sonarcloud bot commented Sep 6, 2023

BobanL left a comment

emyl3 left a comment

johanna-skylight left a comment

replace manage facility admin screen dropdowns with combos #6439

replace manage facility admin screen dropdowns with combos #6439

Conversation

fzhao99 commented Aug 28, 2023 • edited Loading

FRONTEND PULL REQUEST

Related Issue

Changes Proposed

Testing

Before

After

Org selection enables facility selection

Clear filters on individual combos cascade

Clear filter button clears both boxes

Clear org resets search results

Additional Information

The issue

Option 0 - Use fireEvent

Option 1 - Wrap the userEvents in act

Option 2 - Update dependencies that are causing the issue in the first place and refactor our testing suite

Choose a reason for hiding this comment

fzhao99 commented Sep 5, 2023

emyl3 left a comment

Choose a reason for hiding this comment

johanna-skylight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fzhao99 Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Sep 6, 2023

BobanL left a comment

Choose a reason for hiding this comment

emyl3 left a comment

Choose a reason for hiding this comment

johanna-skylight left a comment

Choose a reason for hiding this comment

fzhao99 commented Aug 28, 2023 •

edited

Loading

Option 1 - Wrap the userEvents in `act`

fzhao99 Sep 6, 2023 •

edited

Loading